Fix Pandoc subdocument XML namespace handling - #658
Conversation
yangfan-yf-yf
left a comment
There was a problem hiding this comment.
Requesting changes for two blocking issues:
-
Subdoc._get_xml() now serializes each direct body child independently. lxml writes every inherited namespace declaration onto every child, including namespaces that are not used by that child or are already available from the destination document. In a CPython 3.12.3 / lxml 6.1.1 benchmark with a 10,000-paragraph dynamic subdocument, the intermediate fragment grew from 340,000 bytes at 1f143fb to 11,800,000 bytes at 62e0075; median render() time across three fresh runs grew from 0.083 s to 2.400 s. The same path affects every subdocument, not only Pandoc input. Please preserve only the missing or conflicting declarations needed by the inserted content and add a scale regression.
-
tests/pandoc_subdoc.py is not exercised by the current test job. The job invokes python tests/runtests.py from the repository root, but the runner globs [A-Za-z]*.py relative to the current directory, so it runs setup.py rather than the scripts under tests. It also ignores each subprocess.call() return code. Against 1f143fb, the new regression script exits 1 with the reported namespace error, while runtests.py still exits 0. Please discover tests relative to file and propagate child-process failures, or invoke this regression directly from the test job.
The namespace fix itself handles the reported document correctly: the Pandoc 3.9 reproduction fails at the base with the unbound a prefix and succeeds at this head with the image relationship, a/pic elements, nested section properties, and one final body sectPr intact. Custom default namespaces, prefixed attributes, and conflicting prefix mappings also remained resolved.
|
Thanks for the detailed reproduction. Both requested changes are addressed in
The follow-up also passes the related subdocument/header-footer checks, |
ahmetmusab42-stack
left a comment
There was a problem hiding this comment.
I rechecked exact head 24e1c2f after the serialization and test-gate changes.
- The #620 Pandoc fixture now preserves the DrawingML/PictureML nodes and image relationship, reopens with
python-docx, and leaves exactly one final bodysectPr. - The default-namespace, namespaced-attribute, and conflicting-prefix cases remain resolvable.
- The 10,000-paragraph regression retains all 10,000 paragraphs in a 340,000-byte fragment, avoiding the prior per-child namespace amplification.
- The root-invoked runner discovers and executes
pandoc_subdoc.pyand propagates failures. I also ran the related subdocument, merge, and header/footer scripts and an exact-venv CLI render separately. - Flake8, sdist/wheel builds, and
git diff --checkpassed.
I did not repeat the interactive Word GUI check, but I found no remaining blocker in the XML/package behavior or in the two previously requested changes.
Why
Pandoc-generated DOCX files can declare DrawingML namespaces on the document
root rather than on each body child. Removing the serialized
w:bodywrapperthen leaves unbound prefixes in the inserted fragment, so Word-compatible
parsers cannot reopen the rendered document.
Root cause
The original implementation serialized the whole body and removed the wrapper,
including declarations required by descendants. The first fix serialized every
child independently, which preserved correctness but repeated inherited
namespace declarations on every child and caused severe fragment growth.
Change
conflicting declarations on inserted content
sectPrremoval behaviorsys.executable, propagate child failures, and emit a deterministicSHA-256 RUN/PASS ledger
Validation
regression with an 11,800,000-byte fragment
1,000,000-byte structural bound
relationship, DrawingML/PictureML nodes, namespace conflicts, and final
sectPrplacementsubdocument/header-footer checks, flake8, wheel/sdist builds, and archive
audits in both review phases
without a repair prompt and confirmed the image renders
Scope and risks
This changes body-fragment serialization and the tracked test launcher only.
It does not change public APIs, template syntax, dependencies, relationships,
fixtures, headers, footers, or footnotes. The launcher change ensures this PR's
regression is a real CI-discovered test; broader nested-command cleanup remains
separate from this issue. Remote GitHub Actions still require repository-side
approval to run.
Closes #620